chore: DSPX-1226 use key ids per attribute #634

ntrevino-virtru · 2025-06-18T14:35:35Z

No description provided.

…px-1226/use-key-ids-per-attribute

gemini-code-assist

Summary of Changes

Hello @ntrevino-virtru, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request implements a comprehensive update to the core key management and policy enforcement mechanisms within the system. It introduces new data models and refactors existing client-side logic to provide more explicit control over Key Access Server (KAS) key identification, management, and their association with policy attributes. Additionally, it enhances OpenID Connect (OIDC) integration by allowing configurable endpoints and lays groundwork for advanced key lifecycle management features.

Highlights

Key Management Overhaul: Introduced new SimpleKasKey, PublicKeyCtx, and PrivateKeyCtx types, and updated core policy objects (Namespace, Attribute, Value, KeyAccessServer) to utilize these new, more explicit key representations. This includes changes to how KAS keys are associated with attributes and values.
Enhanced Key Modes: Expanded the KeyMode enum with detailed classifications like CONFIG_ROOT_KEY, PROVIDER_ROOT_KEY, and PUBLIC_KEY_ONLY, providing more granular control over key management strategies.
Configurable OIDC Endpoints: Added the capability to explicitly configure OIDC token and user info endpoints within authentication providers, enhancing flexibility for OIDC integration.
Key Rotation and Base Key Management: Introduced new RPC methods (SetBaseKey, GetBaseKey) and response structures (RotatedResources) in the KAS registry, enabling more robust management and tracking of key rotations.
Client-Side Key Resolution Refactor: The TDF3 client's key resolution logic was significantly refactored to use a caching mechanism for KAS public keys and to correctly handle various public key contexts (remote, cached) during the encryption process, including the use of explicit Key IDs (kid).

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

ntrevino-virtru · 2025-06-18T14:36:30Z

Reated to #622

gemini-code-assist

Code Review

This pull request introduces several significant changes, primarily focused on enhancing OIDC endpoint configurability and refactoring how key identifiers and attributes are handled, particularly within the policy and TDF3 client logic.

Key changes include:

Adding oidcTokenEndpoint and oidcUserInfoEndpoint options for more flexible OIDC integration.
Substantial updates to Protobuf definitions, including new message types (SimpleKasKey, PublicKeyCtx, etc.), changes to existing enums (KeyStatus, KeyMode), and the removal of google.api.http options from many gRPC services. These are potentially breaking changes that need careful consideration and documentation.
Refactoring of the policy granter logic to use KeyAccessServer objects more directly and incorporate kid (Key ID) into the key splitting plan, aligning with the PR's goal of using key IDs per attribute.
Updates to the TDF3 client to support these new KAS key handling mechanisms, including a more detailed splitPlan and caching for KAS public key fetching.

Several critical issues have been identified, primarily related to the Protobuf changes, such as the removal of HTTP/JSON transcoding annotations and a suspicious field name/type swap in RotateKeyRequest_NewKey. These require immediate attention. The PR description should be updated to reflect these significant changes and their potential impact.

lib/src/platform/policy/resourcemapping/resource_mapping_pb.ts

lib/src/platform/policy/kasregistry/key_access_server_registry_pb.ts

lib/src/platform/policy/unsafe/unsafe_pb.ts

lib/src/platform/policy/kasregistry/key_access_server_registry_pb.ts

lib/src/platform/policy/attributes/attributes_pb.ts

lib/src/policy/granter.ts

lib/tdf3/src/client/index.ts

…ey-ids-per-attribute # Conflicts: # lib/src/opentdf.ts # lib/tdf3/src/client/index.ts

c-r33d · 2025-06-23T16:04:27Z

lib/tests/web/policy/mock-attrs.ts

@@ -282,7 +295,7 @@ export function valueFor(attr: string): Value {
  if (!(attr in values)) {
    throw new Error(`invalid FQN [${attr}]`);
  }
-  console.log('value for', attr, 'is', values[attr]);
+  // console.log('value for', attr, 'is', values[attr]);


should we remove this?

c-r33d · 2025-06-23T16:41:49Z

lib/tdf3/src/client/index.ts

-      ? splitPlan
-      : [{ kas: opts.defaultKASEndpoint ?? this.kasEndpoint }];
+    // TODO KAS: check here
+    if (splitPlan.length === 0) {


Probably will have Eugene's base key work here I believe.

c-r33d · 2025-06-23T16:41:59Z

lib/tdf3/src/client/index.ts

-    const splits: SplitStep[] = splitPlan?.length
-      ? splitPlan
-      : [{ kas: opts.defaultKASEndpoint ?? this.kasEndpoint }];
+    // TODO KAS: check here


c-r33d

Am I missing the step where we are adding SimpleKasKeys to the kasKeys cache? The flow should be the following:
An attribute comes back with:

Grants/SimpleKasKeys

If we notice that the response has KasKeys we use those instead of the grants returned, else use the grants.

Example of this in the golang sdk:
https://github.com/opentdf/platform/blob/main/sdk/granter.go#L287-L304

lib/tdf3/src/client/index.ts

lib/src/policy/granter.ts

…px-1226/use-key-ids-per-attribute

…ey-ids-per-attribute

sonarqubecloud · 2025-07-09T15:47:03Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

eugenioenko · 2025-07-09T22:36:31Z

lib/src/policy/granter.ts

+        return [keyHolder, keyParts.join('/')] as [KeyHolder, string];
+      })
+      .sort(([, sortKeyA], [, sortKeyB]) => {
+        return sortKeyA.localeCompare(sortKeyB);


eugenioenko · 2025-07-09T22:40:43Z

lib/tests/web/access/access-fetch.test.ts

really nice 🔥

eugenioenko

Went through the code changes and LGTM

ntrevino-virtru added 6 commits June 10, 2025 09:30

chore: DSPX-1236 allow using proper endpoints for OIDC

b566072

Merge remote-tracking branch 'refs/remotes/origin/main' into chore/ds…

2ab1e4b

…px-1226/use-key-ids-per-attribute

Use platformUrl instead of policyEndpoint

9c1f5ef

Add additional key types.

34324e1

Use updated protos.

65c4a4e

Refactor KAS info cache

1136a16

gemini-code-assist bot reviewed Jun 18, 2025

View reviewed changes

ntrevino-virtru added 2 commits June 20, 2025 16:29

Fix tests.

bd7197f

Merge remote-tracking branch 'origin/main' into chore/dspx-1226/use-k…

3056cfd

…ey-ids-per-attribute # Conflicts: # lib/src/opentdf.ts # lib/tdf3/src/client/index.ts

ntrevino-virtru marked this pull request as ready for review June 23, 2025 15:38

ntrevino-virtru requested a review from a team as a code owner June 23, 2025 15:38

ntrevino-virtru requested a review from dmihalcik-virtru June 23, 2025 15:54

c-r33d reviewed Jun 23, 2025

View reviewed changes

Add fetch tests to support branch coverage.

c206f87

eugenioenko reviewed Jun 23, 2025

View reviewed changes

lib/tdf3/src/client/index.ts Outdated Show resolved Hide resolved

Add keys to cache when they are able to be pre-loaded.

c7e3a2f

eugenioenko reviewed Jun 26, 2025

View reviewed changes

lib/src/policy/granter.ts Outdated Show resolved Hide resolved

ntrevino-virtru added 4 commits June 30, 2025 11:22

Update cache key joining to use separators.

52ccc1a

Merge remote-tracking branch 'refs/remotes/origin/main' into chore/ds…

057f2e3

…px-1226/use-key-ids-per-attribute

Add tests and update key function.

eab1aa5

Merge remote-tracking branch 'origin/main' into chore/dspx-1226/use-k…

09550d9

…ey-ids-per-attribute

eugenioenko reviewed Jul 9, 2025

View reviewed changes

lib/tests/web/access/access-fetch.test.ts

Copy link

Contributor

eugenioenko Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really nice 🔥

eugenioenko approved these changes Jul 9, 2025

View reviewed changes

ntrevino-virtru merged commit b37e736 into main Jul 9, 2025
32 of 36 checks passed

ntrevino-virtru deleted the chore/dspx-1226/use-key-ids-per-attribute branch July 9, 2025 23:05

chore: DSPX-1226 use key ids per attribute #634

chore: DSPX-1226 use key ids per attribute #634

Uh oh!

Conversation

ntrevino-virtru commented Jun 18, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

ntrevino-virtru commented Jun 18, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

c-r33d Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

c-r33d Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

c-r33d Jun 23, 2025

Choose a reason for hiding this comment

Uh oh!

c-r33d left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Jul 9, 2025

Quality Gate passed

Uh oh!

eugenioenko Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

eugenioenko Jul 9, 2025

Choose a reason for hiding this comment

Uh oh!

eugenioenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!